[Core] Support track1 and track2 mgmt SDK side by side in CLI #12952

jsntcy · 2020-04-10T07:06:02Z

Description of PR (Mandatory)
This PR does the following things to support track1 and track2 mgmt SDK side by side:

Integrate azure-mgmt-core 1.0.0
Authentication
- add get_token() callback to support track2
Config
- Use client_kwargs to replace client.config in track2
Error handling
- Also support HttpResponseError AzureError from track2
LRO poller
- Also support LRO poller from track2
Paging
- Also support paging from track2
Remove unexpected arguments as track2 will raise exception when it finds unknown arguments.
- remove raw argument for no wait and pass raw explicitly in method if necessary.
- remove parameters argument for generic update and use actual arguments in SDK instead.

Testing Guide

All CLI automation tests pass.
Use CodeGen v2 to generate CLI extensions to test.

History Notes:
(Fill in the following template if multiple notes are needed, otherwise PR title will be used for history note.)

[Component Name 1] (BREAKING CHANGE:) (az command:) make some customer-facing change.
[Component Name 2] (BREAKING CHANGE:) (az command:) make some customer-facing change.

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.

yonzhan · 2020-04-10T07:55:08Z

add to S168

mmyyrroonn · 2020-04-10T08:19:59Z

src/azure-cli/azure/cli/command_modules/advisor/custom.py

    from msrestazure.azure_exceptions import CloudError

-    response = sdk_no_wait(True, client.generate)
+    response = client.generate(raw=True)


so. Is sdk_not_wait useless?

No, sdk_not_wait is still useful for LRO as it will set "polling" to false if no_wait is true.
But here it should not use sdk_no_wait as it's not a LRO (I checked the SDK method.)
'raw' is another story which is different from LRO, so it should not be set in sdk_no_wait.

All other places I replace 'sdk_no_wait' with 'raw=True' actually are bug fix for those modules.

In reply to: 406657030 [](ancestors = 406657030)

jiasli · 2020-04-10T08:22:04Z

src/azure-cli/azure/cli/command_modules/advisor/custom.py

    from msrestazure.azure_exceptions import CloudError

-    response = sdk_no_wait(True, client.generate)
+    response = client.generate(raw=True)


Maybe we can split the migration of advisor and backup to a different PR?

I can split it to another PR.

In reply to: 406657692 [](ancestors = 406657692)

lmazuel · 2020-04-10T17:52:02Z

src/azure-cli-core/azure/cli/core/util.py

                         "https://docs.microsoft.com/cli/azure/query-azure-cli?view=azure-cli-latest")
            return 1
-        if isinstance(ex, (CLIError, CloudError, AzureException)):
+        if isinstance(ex, (CLIError, CloudError, AzureException, HttpResponseError, AzureError)):


Optional: HttpResponseError being a subclass of AzureError, you could simply catch AzureError

Got it, will update in next commit.

In reply to: 406869816 [](ancestors = 406869816)

lmazuel

Core changes LGTM, won't comment on advisor/backup, as suggested by @jiasli I would do atomic PR and split them, up to you.

qianwens

jsntcy · 2020-04-13T04:21:16Z

src/azure-cli/azure/cli/command_modules/advisor/custom.py

    from msrestazure.azure_exceptions import CloudError

-    response = sdk_no_wait(True, client.generate)
+    response = client.generate(raw=True)


@Prasanna-Padmanabhan,
Here we pass raw=True in method instead of using sdk_no_wait (actually it's not correct to use sdk_no_wait before as client.generate is not a long running operation.), please help confirm this change.

I don't have the necessary knowledge to sign off on this change. If all existing tests are passing, then you can go ahead.

@Prasanna-Padmanabhan, thanks for your confirm. All existing tests are passing, so we'll commit this change.

Juliehzl · 2020-04-13T08:30:23Z

src/azure-cli-core/azure/cli/core/commands/client_factory.py

+    client_kwargs['headers'] = headers
+
+    if 'x-ms-client-request-id' in cli_ctx.data['headers']:
+        client_kwargs['request_id'] = cli_ctx.data['headers']['x-ms-client-request-id']


Some questions here:

request_id is required for client?

Should we use x-ms-request-id?
x-ms-request-id: This is a common response header that contains a unique identifier for the current operation, service generated.
x-ms-client-request-id: Optional. Contains a unique ID provided by the client to identify the specific request.
It looks x-ms-client--request-id is right. #WontFix

Actually request_id will be set to "x-ms-client-request-id" in request header, you can check the code in azure core below:

def on_request(self, request):
# type: (PipelineRequest) -> None
"""Updates with the given request id before sending the request to the next policy.

:param request: The PipelineRequest object :type request: ~azure.core.pipeline.PipelineRequest """ request_id = unset = object() if 'request_id' in request.context.options: request_id = request.context.options.pop('request_id') if request_id is None: return elif self._request_id is None: return elif self._request_id is not _Unset: request_id = self._request_id # type: ignore elif self._auto_request_id: request_id = str(uuid.uuid1()) # type: ignore if request_id is not unset: header = {"x-ms-client-request-id": request_id}

In reply to: 407376852 [](ancestors = 407376852)

jsntcy requested review from Juliehzl, Prasanna-Padmanabhan, arrownj, fengzhou-msft, haroldrandom, jiasli and qianwens as code owners April 10, 2020 07:06

jsntcy changed the title ~~[Core] Support track2 mgmt SDK in CLI~~ [Core] Support track1 and track2 mgmt SDK side by side in CLI Apr 10, 2020

jsntcy added the do-not-merge label Apr 10, 2020

jsntcy requested a review from lmazuel April 10, 2020 07:48

yonzhan assigned jsntcy Apr 10, 2020

yonzhan added this to the S168 milestone Apr 10, 2020

mmyyrroonn reviewed Apr 10, 2020

View reviewed changes

jiasli reviewed Apr 10, 2020

View reviewed changes

lmazuel reviewed Apr 10, 2020

View reviewed changes

lmazuel approved these changes Apr 10, 2020

View reviewed changes

qianwens approved these changes Apr 13, 2020

View reviewed changes

jiasli approved these changes Apr 13, 2020

View reviewed changes

jsntcy commented Apr 13, 2020

View reviewed changes

jsntcy added 10 commits April 13, 2020 14:08

draft: integrate azure-mgmt-core 1.0.0b1

fdd36ad

remove code related to storage track 2

63b645d

use client_kwargs to replace client.config in track2

9b29219

fix style errors

7c94b7d

pass accurate paramters for generic update

01159db

fix test error

369fc72

pass raw=True explicitly for sdk calls

a6e1db4

remove unused import

3788001

use azure-mgmt-core 1.0.0b2

65ec3eb

use azure-mgmt-core 1.0.0

ce3c794

Juliehzl reviewed Apr 13, 2020

View reviewed changes

Juliehzl approved these changes Apr 13, 2020

View reviewed changes

jsntcy added 2 commits April 13, 2020 17:19

remove useless comma

5ca8874

only catch base exception instead both base and sub exceptions

c924cf4

jsntcy force-pushed the azure-core-preview branch from da45cca to c924cf4 Compare April 13, 2020 09:26

remove unused import

5284b0c

yonzhan merged commit 62fdf0e into dev Apr 14, 2020

jiasli mentioned this pull request Nov 13, 2020

{Core} Honor expiresOn, instead of expiresIn #15901

Merged

jiasli deleted the azure-core-preview branch November 24, 2020 08:55

jiasli mentioned this pull request Feb 20, 2025

Support get_token_info protocol #29967

Closed

[Core] Support track1 and track2 mgmt SDK side by side in CLI #12952

[Core] Support track1 and track2 mgmt SDK side by side in CLI #12952

Uh oh!

Conversation

jsntcy commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yonzhan commented Apr 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmazuel left a comment

Choose a reason for hiding this comment

Uh oh!

qianwens left a comment

Choose a reason for hiding this comment

Uh oh!

jsntcy Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Juliehzl Apr 13, 2020 • edited by jsntcy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jsntcy commented Apr 10, 2020 •

edited

Loading

jsntcy Apr 13, 2020 •

edited

Loading

Juliehzl Apr 13, 2020 •

edited by jsntcy

Loading